Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: prove all tx in a block #6

Merged
merged 8 commits into from
Dec 6, 2023
Merged

feat: prove all tx in a block #6

merged 8 commits into from
Dec 6, 2023

Conversation

nikkolasg
Copy link
Collaborator

No description provided.

@nikkolasg nikkolasg self-assigned this Nov 29, 2023
Copy link
Contributor

@stjepangolemac stjepangolemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions but I'd like to reiterate my reasoning here.

If we want to make the proving logic as portable as possible we want to expose it in such a way that the caller controls everything except the proving.

In pseudocode it might look something like this:

prove(block) -> proof

Everything else should be left for the caller to do. At the same time this is still a draft so feel free to largely ignore what I'm saying if you're still in the middle of development.

itertools = "0.12.0"
plonky2 = "0.1.4"
ethers = { version ="2.0.11", default-features = false, features = ["rustls"]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like ethers is being deprecated gakonst/ethers-rs#2667

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but alloy doesn't contain everything I need at the moment.
And it's not deprecated really, they just don't ship new features. (see tweet)

}

impl BlockData {
pub async fn fetch<T: Into<BlockId> + Send + Sync>(blockid: T) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong but to me it feels a bit off to have fetching logic here. I still don't have a clear description of this repo in my head so that might be the problem.

The reason behind this is that I believe we will have a suite of fetching logic for all kinds of data centralized somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I feel you. To me too.
Reason why I put it here is I needed to get some real data, full blocks. I like when things are all integrated so I didn't want to write a separate script for fetching them etc.
The best thing would be that it's given as input to the proof library, as it is not but I want to be able to run independent tests so I still need to have that logic inside tests at the very least. I could put the fetching in a test ?

Or the best thing: probably should live in the distributed query repo actually, as a separate crate so I can depend on it without having to depend on the whole distributed query repo. Or maybe just a single crate we control.

I'm happy in any of these solutions, but probably as a future fix if you don't mind. Happy to hear you thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having this logic in a separate function in tests would make sure the public functions don't do it internally and still allow you to call it very easily and continue getting the data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a stop gap solution, I just included the whole module in a #[cfg(test)] attribute. Wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fine 👍

@nikkolasg nikkolasg mentioned this pull request Dec 1, 2023
@nikkolasg nikkolasg marked this pull request as ready for review December 1, 2023 13:29
Copy link
Contributor

@lopeetall lopeetall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the informative comments and well-chosen names!

@nikkolasg nikkolasg merged commit c5987a7 into main Dec 6, 2023
4 checks passed
@nikkolasg nikkolasg deleted the feat/all_block_tx branch December 6, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants